Add `DataFusionTableWidget::column_visibility` and use it to improve visibility defaults for the partition table #9936

abey79 · 2025-05-09T09:30:12Z

What

Title.

Also fixes the column ordering, which was wrongly based on their id. And rename a bunch of things for consistency.

Current default view:

github-actions · 2025-05-09T09:31:29Z

Web viewer built successfully. If applicable, you should also test it:

I have tested the web viewer

Result	Commit	Link	Manifest
✅	`4ac93d6`	https://rerun.io/viewer/pr/9936	`+nightly` `+main`

^{Note: This comment is updated whenever you push a commit.}

…visibility defaults for the partition table Also fixes the column ordering, which was wrongly based on their id.

Wumpf

lgtm overall!
some code improvement suggestions and questions on exact semantics

crates/store/re_sorbet/src/column_descriptor_ref.rs

Wumpf · 2025-05-19T09:56:21Z

crates/viewer/re_dataframe_ui/src/datafusion_table_widget.rs

+pub type ColumnNameFn<'a> = Option<Box<dyn Fn(&ColumnDescriptorRef<'_>) -> String + 'a>>;
+
+pub type ColumnVisibilityFn<'a> = Option<Box<dyn Fn(&ColumnDescriptorRef<'_>) -> bool + 'a>>;


type names are a bit misleading imho. Shouldn't this be

Suggested change

pub type ColumnNameFn<'a> = Option<Box<dyn Fn(&ColumnDescriptorRef<'_>) -> String + 'a>>;

pub type ColumnVisibilityFn<'a> = Option<Box<dyn Fn(&ColumnDescriptorRef<'_>) -> bool + 'a>>;

pub type ColumnNameFn<'a> = Box<dyn Fn(&ColumnDescriptorRef<'_>) -> String + 'a>;

pub type ColumnVisibilityFn<'a> =Box<dyn Fn(&ColumnDescriptorRef<'_>) -> bool + 'a>;

These types are basically to reduce the noise in the struct definition (which clippy complained about). They are not public (I removed that erroneous pub qualifier btw). The actual public interface is the builder method (which spell the entire signature for clarity, since they are simple enough). So since this whole option-box thing is used twice (here and in the delegate), I'd rather leave it as is.

crates/store/re_log_types/src/path/entity_path_part.rs

crates/viewer/re_redap_browser/src/servers.rs

crates/viewer/re_dataframe_ui/src/datafusion_table_widget.rs

emilk · 2025-05-19T13:10:08Z

crates/viewer/re_redap_browser/src/servers.rs

+                    desc.short_name(),
+                    RECORDING_LINK_FIELD_NAME
+                        | DATASET_MANIFEST_ID_FIELD_NAME
+                        | DATASET_MANIFEST_REGISTRATION_TIME_FIELD_NAME
+                )


Matching on string here is extremely brittle and is already causing problems with #9983

Well, that's a strange comment to make tbh. This code uses short_name, which #9983 removed, hence the compilation error. What is brittle is our lack of standard around column names and how they relate to descriptors—see #9840. I've pushed a commit on #9983 which replaces short_name by display_name and tested that it still works as intended.

But does it make sense for this code to break when we change how columns are displayed (e.g. when we change the implementation of fn display_name)? Wouldn't it be better to explicitly match against e.g. ColumnDescriptor::archetype_field_name ?

These columns don't have much metadata—basically just a raw record batch built ad hoc on the server. Specifically, they don't have an archetype. Again, we need a much strong system to relate record batch/dataframe column name with column descriptors, which must account for arbitrary tables sent by the server and/or users.

What is Name in the above screenshot? Is that ComponentName? If so, can we match against that instead of display_name?

For component column, it corresponds to component_name (this view is a mirror of the fields of ComponentColumnDescriptor). That field is currently only exposed indirectly in ColumnDescriptorRef's methods. Doing it properly amounts to resolving the desc <-> column name problem.

emilk · 2025-05-19T13:11:35Z

crates/viewer/re_redap_browser/src/servers.rs

@@ -115,6 +117,8 @@ impl Server {
        ui: &mut egui::Ui,
        dataset: &Dataset,
    ) {
+        const RECORDING_LINK_FIELD_NAME: &str = "recording link";


What is this hard-coded string? Where does it come from?

This is a column that is live injected using datafusion.

Where is it live-injected? Who gives it this name? What other piece of code needs to be updated in tandem with this piece of code?

As the location of this const suggests, this is all local to this method. generate_partition_links is what triggers that column generation (it takes a column name as input), and default_column_visibility defines, well, what the columns are visible by default (and it wants that generated column to be visible by default).

abey79 added ui concerns graphical user interface exclude from changelog PRs with this won't show up in CHANGELOG.md dataplatform Rerun Data Platform integration feat-redap-browser Everything related to the in-viewer Redap browser labels May 9, 2025

abey79 marked this pull request as ready for review May 9, 2025 09:55

abey79 force-pushed the antoine/partition-table-col-viz branch from ef10d2c to 3d49ec9 Compare May 9, 2025 14:46

Wumpf self-requested a review May 9, 2025 16:08

abey79 added 3 commits May 9, 2025 19:29

Add DataFusionTableWidget::column_visibility and use it to improve …

3d45e63

…visibility defaults for the partition table Also fixes the column ordering, which was wrongly based on their id.

Post merge fix

2bdcda1

rustfmt

5f500eb

abey79 force-pushed the antoine/partition-table-col-viz branch from 3d49ec9 to 5f500eb Compare May 9, 2025 17:31

Merge branch 'main' into antoine/partition-table-col-viz

19d73de

Wumpf approved these changes May 19, 2025

View reviewed changes

Review comments

4ac93d6

abey79 merged commit a3ddd26 into main May 19, 2025
37 checks passed

abey79 deleted the antoine/partition-table-col-viz branch May 19, 2025 12:25

emilk reviewed May 19, 2025

View reviewed changes

		pub type ColumnNameFn<'a> = Option<Box<dyn Fn(&ColumnDescriptorRef<'_>) -> String + 'a>>;

		pub type ColumnVisibilityFn<'a> = Option<Box<dyn Fn(&ColumnDescriptorRef<'_>) -> bool + 'a>>;

Add DataFusionTableWidget::column_visibility and use it to improve visibility defaults for the partition table #9936

Add DataFusionTableWidget::column_visibility and use it to improve visibility defaults for the partition table #9936

Uh oh!

Conversation

abey79 commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related

What

Uh oh!

github-actions bot commented May 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Wumpf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emilk May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abey79 May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Add `DataFusionTableWidget::column_visibility` and use it to improve visibility defaults for the partition table #9936

Add `DataFusionTableWidget::column_visibility` and use it to improve visibility defaults for the partition table #9936

abey79 commented May 9, 2025 •

edited

Loading

github-actions bot commented May 9, 2025 •

edited

Loading

emilk May 19, 2025 •

edited

Loading

abey79 May 19, 2025 •

edited

Loading